-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add wrapper for linux kernel module loading #930
Conversation
bachp
commented
Jul 20, 2018
- init_module and finit_module to load kernel modules
- delete_module to unload kernel modules
src/kmod.rs
Outdated
/// init_module(&mut contents, &CString::new("").unwrap()).unwrap(); | ||
/// ``` | ||
/// | ||
pub fn init_module(module_image: &[u8], param_values: &CStr) -> Result<i64> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more flexible way then passing the binary as &[u8]
?
src/lib.rs
Outdated
@@ -43,6 +43,8 @@ pub mod fcntl; | |||
target_os = "openbsd"))] | |||
pub mod ifaddrs; | |||
#[cfg(any(target_os = "linux", target_os = "android"))] | |||
pub mod kmod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the right place to put this nor if this is the proper name. Inputs are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good overall, but it could use some tests. Is there a trivial kernel module that you could use for testing? It's ok to have tests that can only run as root; we already have a few.
@asomers What is the best way to run tests as root? Should I run all of the build as root or is there a better way? |
Do it just like the |
@asomers I added some tests |
test/test_kmod/hello_mod/hello-1.c
Outdated
printk(KERN_INFO "Goodbye world 1.\n"); | ||
} | ||
|
||
MODULE_LICENSE("GPL"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why GPL? The rest of Nix uses MIT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module is taken from tldp.org where it is licensed GPL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also isn't specific enough, as the GPL has versions. I assume that means 2.0. Let's get a new one written that we can license MIT. Also, can we name it hello.c
. That -1
is really killing me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A pure MIT licensed module would taint the kernel if loaded. The option I see would be: Dual MIT/GPL
. I willl create a new module and license it accordingly.
test/test_kmod/mod.rs
Outdated
use std::process::Command; | ||
|
||
fn compile_kernel_module() -> String { | ||
#[allow(unused_variables)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The #[allow(unused_variables)]
syntax is holdover from an older version of rustc. Nowadays you can just do let _, = ...
and it won't complain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it and it doesn't work. I'm not sure what's the scope of _
but it seems it doesn't stick around till the end of the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my last comment had a typo. You need to use let _m = ...
, not let _ = ...
. There is a difference between _m
and _
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. I didn't know they behave differently. I update the code.
On my Debian system the test_finit_and_delete_module test fails with this error:
|
@asomers You most probably don't have the kernel sources installed on your system. Building a kernel module requires the kernel source. I'm not sure what packages provides these on Debian. |
I installed the linux-source package, and now I have sources in /usr/src, but I still get the same error. The fact that the error message mentions a "build" directory suggests that maybe there's a missing step? |
I tried on a debian system and the required package is You can install it via:
|
Ok, installing linux-headers did the trick. But now "cargo test" is trying to create some temporary files in nix's test directory. Could you fix it so those are created in TMPDIR instead?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to see the kernel module we use allow for parameters to be passed. I don't see those tested as part of this, and I think it's important that it get tested, ideally having 2 parameters or more.
test/test_kmod/hello_mod/hello-1.c
Outdated
printk(KERN_INFO "Goodbye world 1.\n"); | ||
} | ||
|
||
MODULE_LICENSE("GPL"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also isn't specific enough, as the GPL has versions. I assume that means 2.0. Let's get a new one written that we can license MIT. Also, can we name it hello.c
. That -1
is really killing me.
test/test_kmod/hello_mod/Makefile
Outdated
@@ -0,0 +1,7 @@ | |||
obj-m += hello-1.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make this hello.o
test/test.rs
Outdated
mod sys; | ||
mod test_fcntl; | ||
#[cfg(any(target_os = "linux", target_os = "android"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please alphabetize the OSes
src/lib.rs
Outdated
@@ -43,6 +43,8 @@ pub mod fcntl; | |||
target_os = "openbsd"))] | |||
pub mod ifaddrs; | |||
#[cfg(any(target_os = "linux", target_os = "android"))] | |||
pub mod kmod; | |||
#[cfg(any(target_os = "linux", target_os = "android"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please alphabetize here.
src/lib.rs
Outdated
@@ -43,6 +43,8 @@ pub mod fcntl; | |||
target_os = "openbsd"))] | |||
pub mod ifaddrs; | |||
#[cfg(any(target_os = "linux", target_os = "android"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please alphabetize this.
src/kmod.rs
Outdated
|
||
/// Loads a kernel module from a given file descriptor. | ||
/// | ||
/// Example usage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the above, use # Example
instead.
src/kmod.rs
Outdated
libc_bitflags!( | ||
/// Flags used by `delete_module`. | ||
pub struct OFlags: libc::c_int { | ||
O_NONBLOCK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add doccomments for these items.
src/kmod.rs
Outdated
|
||
libc_bitflags!( | ||
/// Flags used by `delete_module`. | ||
pub struct OFlags: libc::c_int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this to OutputFlags
or something less short-hand?
src/kmod.rs
Outdated
|
||
/// Unloads the kernel module with the given name. | ||
/// | ||
/// Example usage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Example
Did you try simply cd'ing to ${TMPDIR:-/tmp} before running make? |
@asomers I don't think switching the workdir before doing make would work as the workdir needs to be in the same directory. One option would be to copy everything to a TMP location, do the build and then delete it again after the test. But it seems hacky. |
@bachp could you try that then? No test should be creating files in the source directory. I can't even run these tests, because my source directory is mounted over NFS with rootsquash on. |
8847d62
to
9eede2c
Compare
@asomers I bumped the minimum required rust version to 1.24.1 as this is the version packaged in Debian. So it should allow them to still package rust tools depending on nix. I think the freebsd jobs should be updated to this version too. |
@Susurrus I incorporated your feedback into the documentation and added some additional information and links. |
Why 1.24.1? If Nix only depends on 1.22.0 features, then the minimum rustc version should be 1.22.0. That shouldn't be a problem for building Debian packages. Nor should Nix depend in any way on Debian. |
@asomers Rust 1.22 produces some compile errors that are not present in 1.20 and are no longer present in 1.24.1. See: https://travis-ci.org/nix-rust/nix/builds/413801114 for details on the error. |
That looks like a bug in Nix. |
But why does this warning only pop up with 1.22 and not with 1.24.1 or 1.28? |
I updated the MR to build with 1.22.1 and changed the offending code. But travis seems to have failed for some other reason now. Seems unrelated to the change. |
The Travis failures were spurious. Restarting the jobs fixed them. I'm upgrading buildbot now. |
Anything else that needs to be fixed? |
CHANGELOG.md
Outdated
|
||
### Changed | ||
- Bump minimum tested Rust version to 1.22.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"tested" -> "required"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit/change will be removed once #900 is merged
e664162
to
b6eb166
Compare
Rebased after #900 got merged |
I rebased again to fix the changelog conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me, so I think it's time to refactor the commits and then we can merge. Please refactor this PR into 4 commits in the following order:
- Update rand crate
- Remove
allow(unused_variables)
- Factor out skipping tests if not root into a macro
- Adding the kmod module
This avoids having both 0.4 and 0.5 (required by tempfile)
This macro can be used in tests to skip the test if it requires root to sucssfully run.
- init_module and finit_module to load kernel modules - delete_module to unload kernel modules Signed-off-by: Pascal Bach <pascal.bach@nextrem.ch>
@Susurrus PR reworked into 4 logical commits. |
Anything still missing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @Susurrus do you have any more concerns?
bors r+ |